Skip to content

Revert "[wingman -> rllib] IMPALA MultiDiscrete changes (#3967)"#4332

Merged
ericl merged 1 commit intoray-project:masterfrom
ericl:revert-3967
Mar 12, 2019
Merged

Revert "[wingman -> rllib] IMPALA MultiDiscrete changes (#3967)"#4332
ericl merged 1 commit intoray-project:masterfrom
ericl:revert-3967

Conversation

@ericl
Copy link
Contributor

@ericl ericl commented Mar 12, 2019 •
edited
Loading

What do these changes do?

There seems to be a bug here, isolated to the changes in vtrace.py. Reverting that file and patching up multi_from_logits to call from_logits fixes the regression. Unfortunately I don't have time to investigate this further so we should revert this change and re-land it once the bug is found.

Interestingly, vtrace.py still passes its original unit tests.

The regression is fairly straightforward to reproduce: on any GPU machine run:

atari-impala:
    env:
        grid_search:
            - BreakoutNoFrameskip-v4
    run: IMPALA
    config:
        sample_batch_size: 50
        train_batch_size: 500
        num_workers: 32
        num_envs_per_worker: 5
        clip_rewards: True
        lr_schedule: [
            [0, 0.0005],
            [20000000, 0.000000000001],
        ]

Within a few minutes it gets to 1m+ timesteps. Before the regression, you can expect 10+ reward at this point. After the regression, the reward will top out at 1-2.

Related issue number

Atari performance regression originally reported in #4329

@AmplabJenkins
Copy link

AmplabJenkins commented Mar 12, 2019

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/36/
Test PASSed.

@ericl
Copy link
Contributor Author

ericl commented Mar 12, 2019 

First time build. Skipping changelog.
[Ray-Perf-Integration-PRB] $ /bin/bash /tmp/hudson4942004597160488522.sh
PR 4332 does not contain ray-core, early exiting
Test PASSed.

@robertnishihara is this misdetection due to a revert or is it some other bug?

@pcmoritz
Copy link
Contributor

pcmoritz commented Mar 12, 2019

@ericl This is the new performance testing jenkins, which only kicks in if the PR string contains ray-core afaik (@devin-petersohn), i.e. if something performance-related has changed. ray-core should probably be changed to core.

@ericl
Copy link
Contributor Author

ericl commented Mar 12, 2019

Ah ok, didn't realize it had a different name.

hartikainen
hartikainen reviewed Mar 12, 2019
View reviewed changes
python/ray/rllib/agents/impala/vtrace.py
log_rhos = get_log_rhos(target_action_log_probs,
behaviour_action_log_probs)

log_rhos = target_action_log_probs - behaviour_action_log_probs
Copy link
Contributor

@hartikainen hartikainen Mar 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One potential source of the bug is if this guy has a wrong shape.

Copy link
Contributor Author

@ericl ericl Mar 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the shapes here and it looked reasonable (at least they match).

@AmplabJenkins
Copy link

AmplabJenkins commented Mar 12, 2019

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/12765/
Test FAILed.

@ericl ericl merged commit 3c41cb9 into ray-project:master Mar 12, 2019
stefanpantic added a commit to wingman-ai/ray that referenced this pull request Mar 12, 2019
@stefanpantic
Revert "Revert "[wingman -> rllib] IMPALA MultiDiscrete changes (ray-…
a63e581 
…project#3967)" (ray-project#4332)"

This reverts commit 3c41cb9.
@stefanpantic stefanpantic mentioned this pull request Mar 12, 2019
Merged
@stefanpantic
Copy link
Contributor

stefanpantic commented Mar 12, 2019

@ericl Hi Eric, I think I fixed the issue, I've opened a pull request here

ericl pushed a commit that referenced this pull request Mar 13, 2019
@stefanpantic @ericl
Fix multi discrete (#4338)
2202a81 
* Revert "Revert "[wingman -> rllib] IMPALA MultiDiscrete changes (#3967)" (#4332)"

This reverts commit 3c41cb9.

* Fix a bug with log rhos for vtrace

* Reformat

* lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hartikainen hartikainen hartikainen approved these changes

Assignees

@hartikainen hartikainen

Labels

regression

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ericl @AmplabJenkins @pcmoritz @stefanpantic @hartikainen